Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Expired TWAP and not started TWAP #153

Merged
merged 14 commits into from
Aug 30, 2023
Merged

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Aug 29, 2023

This PR

Includes some optional blockInfo params

This params allow the poll method to use either the blockNumber or the blockTimestamp to validate the concrete Conditional Order.

image

Validate start time

It will check if the order started. If it haven't then we can signal we don't want to re-check until the start time

Handle Expired TWAPs

This is causing a lot of noise in the logs. Expired TWAP orders are never deleted. Now they are
image

Tried it in WatchTower (adapted it in cowprotocol/watch-tower#16)

image

Don't fail to handle errors

Handled errors should not show as errors

image

Not included

Left some notes for a future PR on something that will help to reduce significantly the number of checks we need to do.
It's out of scope cause is not a trivial change. It will need a model update in watch tower
image

@github-actions
Copy link

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

@coveralls
Copy link
Collaborator

coveralls commented Aug 29, 2023

Coverage Status

coverage: 72.617% (-3.8%) from 76.379% when pulling 78c24e3 on smarter-twap-indexing into 34fbd9a on main.

@anxolin anxolin changed the title Smarter twap indexing Handle Expired TWAP and not started TWAP Aug 29, 2023
@anxolin anxolin requested review from mfw78 and a team August 29, 2023 19:20
@anxolin anxolin marked this pull request as ready for review August 29, 2023 19:20
@socket-security
Copy link

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: @cowprotocol/[email protected]

Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Only some nits about assertions / typos.

src/composable/utils.ts Outdated Show resolved Hide resolved
src/composable/ConditionalOrder.ts Outdated Show resolved Hide resolved
src/composable/orderTypes/Twap.ts Outdated Show resolved Hide resolved
src/composable/ConditionalOrder.ts Outdated Show resolved Hide resolved
src/composable/orderTypes/Twap.ts Outdated Show resolved Hide resolved
@anxolin anxolin force-pushed the smarter-twap-indexing branch from de95d05 to 9f59dde Compare August 30, 2023 11:39
@anxolin anxolin force-pushed the smarter-twap-indexing branch from 9f59dde to 4a11e0f Compare August 30, 2023 11:41
@anxolin
Copy link
Contributor Author

anxolin commented Aug 30, 2023

Merging as this is already too big. Lets keep iterating

@anxolin anxolin merged commit 3a61922 into main Aug 30, 2023
4 of 5 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2023
@alfetopito alfetopito deleted the smarter-twap-indexing branch August 31, 2023 08:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants